Skip to content

fix(exec) fail close on invalid deny pattern#781

Open
afjcjsbx wants to merge 2 commits intosipeed:mainfrom
afjcjsbx:fix/fail-close-invalid-deny-pattern
Open

fix(exec) fail close on invalid deny pattern#781
afjcjsbx wants to merge 2 commits intosipeed:mainfrom
afjcjsbx:fix/fail-close-invalid-deny-pattern

Conversation

@afjcjsbx
Copy link

📝 Description

This PR fixes a critical "Fail-Open" security vulnerability in the ExecTool initialization process and improves overall error handling.

Previously, if a custom deny pattern provided in the configuration contained a regex syntax error, the NewExecToolWithConfig function would log the error via fmt.Printf and execute a continue statement. This caused the system to silently discard the malformed security rule and start the agent anyway, leaving it exposed to the commands that were supposed to be blocked.

Changes introduced:

  • Modified NewExecToolWithConfig and NewExecTool to return (*ExecTool, error).
  • Removed the continue statement during regex compilation. If a security pattern fails to compile, the initialization now correctly returns an error, adopting a strict Fail-Closed security posture.
  • Updated tool registration (e.g., NewCronTool and toolsRegistry.Register) to properly handle and bubble up the initialization error, preventing the application from starting in an insecure state.

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

📚 Technical Context (Skip for Docs)

  • Reference URL: N/A
  • Reasoning: Security systems must default to "Fail-Closed". Silently dropping invalid blocklist rules creates a false sense of security where administrators believe a tool (e.g., netcat) is blocked, but the underlying system is completely open due to a simple typo in a configuration file.

🧪 Test Environment

Hardware: Apple Silicon (Mac M-series)
OS: macOS
Model/Provider: Generic
Channels: All

Click to view Logs/Screenshots

Scenario: An administrator attempts to block netcat but makes a typo in the regex (missing closing parenthesis).

"exec": {
  "enable_deny_patterns": true,
  "custom_deny_patterns": ["\b(nc|netcat\b"]
}

❌ BEFORE THIS PR: The picoclaw assistant would start up normally, ignoring the malformed pattern. The command nc or netcat would remain executable, completely bypassing the intended security measure.

✅ AFTER THIS PR:
The system adopts a Fail-Closed approach. The application halts immediately on startup, notifying the user of the broken security configuration:

🔍 Debug mode enabled
Using custom deny patterns: (nc|netca]
2026/02/25 18:22:06 Critical error: unable to initialize exec tool: invalid custom deny pattern "\b(nc|netcat\b": error parsing regexp: missing closing ): (nc|netca`
exit status 1

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly.

@xiaket xiaket requested a review from lxowalle February 26, 2026 09:41
@xiaket xiaket added type: bug Something isn't working domain: tool labels Feb 26, 2026
Copy link
Collaborator

@xiaket xiaket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@PixelTux PixelTux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: tool type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants